Repository navigation
Acknowledge HTTP/2 data received for an already closed stream - #391
Open
mayuriphad wants to merge 1 commit into
Open
mayuriphad wants to merge 1 commit into
mayuriphad wants to merge 1 commit into
Conversation
If an app sends its response without reading the whole request body, the client keeps sending the rest of the body. Once the stream has been closed, the next DataReceived event raised a KeyError, which stopped the connection's reader and left every later request on that connection hanging. Drop the data, as the StreamEnded branch already does, but still acknowledge it so the connection-level flow-control window is restored. Fixes pgjones#389 Co-authored-by: iker-gonzalez <84924263+iker-gonzalez@users.noreply.github.com> Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #389
When an ASGI app responds without reading the whole request body, the client keeps sending the rest of the body over HTTP/2. Once Hypercorn has closed the stream, the next
DataReceivedevent for it hitsself.streams[event.stream_id]and raisesKeyError. That error escapesTCPServer._read_data, so the connection's reader stops while the socket stays open, and every later request on that connection hangs. Behind a proxy that multiplexes many clients over one h2c connection, one early-answered upload stalls unrelated requests.The
StreamEndedbranch right below already ignores a stream that is gone.DataReceivednow does the same, but it still callsacknowledge_received_data, so the connection-level flow-control window is given back and the client can keep sending on other streams.The fix is the one @iker-gonzalez suggested in the issue, so they are credited as co-author.
Tests
test_protocol_data_received_after_stream_closedintests/protocol/test_h2.py: it closes the stream after the request headers, sends three DATA frames for it, and checks that no error is raised and that the client gets a connection-levelWindowUpdated. It fails withKeyError: 1without the change.mainon Windows, and the new test passes.black -l 100,isortandflake8are clean on the changed files.